Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[systemverilog] Add keywords #2635

Merged
merged 2 commits into from
Sep 6, 2020

Conversation

hirooih
Copy link
Contributor

@hirooih hirooih commented Sep 4, 2020

This is a fix for some of the issues I reported on #2618.

  • mark as a Verilog keyword: unsigned
  • add to KeywordTable[]
    • K_REGISTER: chandle, var: register
    • K_IGNORE: const, packed, protected, rand, randc
  • mark sequence as K_PROPERTY
  • add K_MODPORT on hasSimplePortList() to support port lists

@masatake
Copy link
Member

masatake commented Sep 4, 2020

You can run test cases locally with:

make units LANGUAGES=SystemVerilog

@hirooih
Copy link
Contributor Author

hirooih commented Sep 5, 2020

make units LANGUAGES=SystemVerilog

I run now and one fail is detected.

The fail is caused by input and output ports of modports. It is a result as I made them to be detected.

  • add K_MODPORT on hasSimplePortList() to support port lists

But I found that they should be ignored. I will revert the fix.

In addition you may want to write a test case specialized to your change for the code.

I will also add test cases for the issues I fixed.

hirooih added a commit to hirooih/ctags that referenced this pull request Sep 5, 2020
@coveralls
Copy link

coveralls commented Sep 5, 2020

Coverage Status

Coverage remained the same at 86.922% when pulling b8bd77c on hirooih:add-keywords-PR2618 into 36a6dd1 on universal-ctags:master.

@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #2635 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2635   +/-   ##
=======================================
  Coverage   86.82%   86.82%           
=======================================
  Files         183      183           
  Lines       38949    38949           
=======================================
  Hits        33817    33817           
  Misses       5132     5132           
Impacted Files Coverage Δ
parsers/verilog.c 97.47% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36a6dd1...b8bd77c. Read the comment docs.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

LGTM.
As far as reading your change, the change can be squashed into one commit or two commits (one for code change and another for adding a test case.) Could you do this? I would like you to do push --force the squashed one.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

I woud like you to change the commit log like:

SystemVerilog: ...

for changing the code.

Units: ...

for changing/adding test cases.

@hirooih
Copy link
Contributor Author

hirooih commented Sep 5, 2020

I added Units tests for this pull-request.

  • add K_MODPORT on hasSimplePortList() to support port lists

I reverted this fix as I wrote above.

  • mark as a Verilog keyword: unsigned

I also reverted this.
unsigned is a keyword in Verilog. But I found that it could not be used as signed as in SystemVerilog by reading Verilog 2005 LRM.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

Please, see my comments.
#2635 (comment)
and
#2635 (comment)
.

hirooih added a commit to hirooih/ctags that referenced this pull request Sep 5, 2020
@masatake
Copy link
Member

masatake commented Sep 5, 2020

You can remove 3c03141 and a52f96e together from this pull request.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

You can squash d62bab0 and d62bab0 into 2aefcc4.

@hirooih
Copy link
Contributor Author

hirooih commented Sep 5, 2020

I would like you to change the commit log like: ...

I've done this by reading https://docs.github.com/ja/github/committing-changes-to-your-project/changing-a-commit-message.
It seems that I succeed.

As far as reading your change, the change can be squashed into one commit or two commits (one for code change and another for adding a test case.) Could you do this? I would like you to do push --force the squashed one.

Sorry, I'm not sure what you mean. I have little experience with git.

You can remove 3c03141 and a52f96e together from this pull request.
You can squash d62bab0 and d62bab0 into 2aefcc4.

How do I do this? Could you give a reference?
Thank you.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

Sorry, I'm not sure what you mean. I have little experience with git.

I see.
'git rebase -i' may help you.
I found some articles about the command line:

https://hackernoon.com/beginners-guide-to-interactive-rebasing-346a3f9c3a6d
https://medium.com/swlh/tutorial-on-interactive-rebasing-e3f49505b2ce

I can do what I requested to you in place of you. However, I would like to know "git rebase -i" because I expect you send more pull requests in the future:-)

So could you try to learn the command line? If things don't go well, let me know. I will take over this pull request and merge this. Thank you.

@masatake
Copy link
Member

masatake commented Sep 5, 2020

@hirooih
Copy link
Contributor Author

hirooih commented Sep 5, 2020

Thank you for references. I will try them tomorrow.

hirooih added a commit to hirooih/ctags that referenced this pull request Sep 6, 2020
@hirooih
Copy link
Contributor Author

hirooih commented Sep 6, 2020

I've done.

It was much easier than I thought. I used "git rebase -i" to change commit messages, but I never thought it was a such powerful tool.

@masatake
Copy link
Member

masatake commented Sep 6, 2020

The new keywords, packed and protected are not in the test case. Is this o.k.?

@hirooih
Copy link
Contributor Author

hirooih commented Sep 6, 2020

Hi. Good point.
I've been enjoying hacking verilog.c today.
If my current understanding is correct, packed and protected are harmless but not required.
Shall I remove them?

@masatake
Copy link
Member

masatake commented Sep 6, 2020

Shall I remove them?

Yes.

If my current understanding is correct, packed and protected are harmless but not required.
Adding such harmless keywords are acceptiable but I would like you to make a separate commit for
adding them with description.

keywords added: chandle, const, rand, randc, var
mark sequence as K_PROPERTY
@hirooih
Copy link
Contributor Author

hirooih commented Sep 6, 2020

I've pushed the fix.
Thanks.

@masatake masatake merged commit ea8b8b6 into universal-ctags:master Sep 6, 2020
@masatake
Copy link
Member

masatake commented Sep 6, 2020

Thank you for your contribution.

@hirooih hirooih deleted the add-keywords-PR2618 branch September 8, 2020 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants